Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Admin UI for shipping methods - stock locations association #3624

Conversation

cedum
Copy link
Contributor

@cedum cedum commented May 15, 2020

This introduces the admin UI allowing to associate a shipping method to specific stock locations.
This makes editable also Spree::ShippingMethod#available_to_all from admin, being an attribute directly related to stock locations association behavior.

This feature is mostly used when a shipping method must be available to a specific subset of stock locations.
The shipping methods for a package are queried using the following methods:

The second query method filters the shipping methods having available_to_all enabled OR having the stock location associated.
By disabling available_to_all it becomes possible to make specific shipping methods available for specific stock locations.

Screenshot 2020-05-15 at 11 45 13

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@cedum cedum marked this pull request as ready for review May 15, 2020 10:33
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Dumi, this looks great!

@kennyadsl kennyadsl added Important Change changelog:solidus_backend Changes to the solidus_backend gem labels May 15, 2020
@aldesantis
Copy link
Member

@cedum thanks, this is awesome! I was wondering if we can easily clear and disable the Stock Locations field when Available to all is checked? It would be a nice UX addition.

@cedum cedum force-pushed the shipping-methods-locations-assoc-admin-ui branch from 5e1f7bb to 90c43e9 Compare May 22, 2020 11:54
@cedum
Copy link
Contributor Author

cedum commented May 22, 2020

I've introduced a few interactions between the checkbox and the select:

  • when checking the checkbox, the selected stock locations get cleared;
  • when selecting a stock location, the checkbox gets unchecked automatically;
  • when clearing all stock locations, the checkbox gets checked automatically.

What do you think?

Here's a demo:

@aldesantis
Copy link
Member

@cedum maybe it's not a good idea to auto-select the checkbox when clearing all stock locations? I'm thinking about a scenario where a store owner wants to temporarily disable a shipping method by removing it from all stock locations. They may not notice that the "Available to all stock locations" checkbox got auto-selected, and they would have the opposite result of what they were expecting. 😄

@cedum
Copy link
Contributor Author

cedum commented May 22, 2020

@cedum maybe it's not a good idea to auto-select the checkbox when clearing all stock locations? I'm thinking about a scenario where a store owner wants to temporarily disable a shipping method by removing it from all stock locations. They may not notice that the "Available to all stock locations" checkbox got auto-selected, and they would have the opposite result of what they were expecting. 😄

I think the scope of this feature is to support the main two cases UI-wise:

  • adding a stock locations whitelist;
  • removing the stock locations whitelist.

Actually, I'm thinking that the "Available to all" checkbox may be completely removed 😄 We can switch that attribute via a hidden input, it's mostly an implementation detail which probably should not be exposed outside.

Disabling a shipping method by removing the stock locations whitelist and disabling the checkbox IMHO is an edge-case, and I think it may be a surprise for a user. If there's a need for such a feature, we may think about introducing a dedicated and clear action for that.

What do you think? Not being a heavy Solidus admin user I may have misinterpreted something 😄

@aldesantis
Copy link
Member

I think it makes sense — if the main use case is the whitelist, we can remove the user-facing "Available to all stock locations" input and just add a hint under the "Stock locations" input.

@cedum cedum force-pushed the shipping-methods-locations-assoc-admin-ui branch from 90c43e9 to 5bd9fa1 Compare May 29, 2020 16:02
@cedum
Copy link
Contributor Author

cedum commented May 29, 2020

When trying to transform the "Available to all" checkbox into a hidden input
"switched" on/off via JS, I've realized it might cause inconsistencies in case of admin JS errors blocking the hidden input update, hence being difficult to debug/understand.

I took and ported @DanielePalombo suggestion from an old PR he was experimenting with. The Daniele's approach I think is better and way less confusing.

Here's a demo, hopefully, the last one 😅

@cedum cedum force-pushed the shipping-methods-locations-assoc-admin-ui branch from 5bd9fa1 to f94d107 Compare May 29, 2020 16:42
Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @cedum, just one small nitpick on the wording for the checkbox hint!

core/config/locales/en.yml Outdated Show resolved Hide resolved
@cedum cedum force-pushed the shipping-methods-locations-assoc-admin-ui branch from f94d107 to bcc8733 Compare June 4, 2020 07:18
Introduce the admin UI to be able to associate a shipping method to
specific stock locations.
This makes editable also `Spree::ShippingMethod#available_to_all` from
admin, being directly related to stock location association behaviour.
@cedum cedum force-pushed the shipping-methods-locations-assoc-admin-ui branch from bcc8733 to 570a16a Compare June 5, 2020 06:46
@cedum cedum requested a review from aldesantis June 5, 2020 08:04
@aldesantis
Copy link
Member

This is amazing, thanks @cedum!

@aldesantis aldesantis merged commit 3724171 into solidusio:master Jun 5, 2020
@aldesantis aldesantis deleted the shipping-methods-locations-assoc-admin-ui branch June 5, 2020 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants